Use NWPathMonitor for modern Apple reachability with legacy fallback#1431
Open
bmehta001 wants to merge 17 commits into
Open
Use NWPathMonitor for modern Apple reachability with legacy fallback#1431bmehta001 wants to merge 17 commits into
bmehta001 wants to merge 17 commits into
Conversation
Replace the deprecated SCNetworkReachability APIs with NWPathMonitor for modern Apple deployment targets (iOS 12+, macOS 10.14+). The legacy SCNetworkReachability path is retained behind a compile-time check for older targets. Changes: - NetworkInformationImpl.mm: refactor to use NWPathMonitor as the primary reachability mechanism, with SCNetworkReachability as fallback for older deployment targets only - ODWReachability.h/m: add NWPathMonitor-based implementation gated on availability, keeping SCNetworkReachability for backward compatibility - Remove dead private header imports from tests This eliminates the -Wdeprecated-declarations build failures on Xcode 26.4+ without needing pragma suppressions. Fixes microsoft#1425 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
PR microsoft#1431 should not carry changes in ODWReachabilityTests.mm. Restore the socket header imports so the branch only contains the reachability implementation changes. Files changed: - tests/unittests/obj-c/ODWReachabilityTests.mm Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep PR microsoft#1431 focused on the reachability implementation changes by reverting the top-of-file/header-area edits in ODWReachability.m. Files changed: - third_party/Reachability/ODWReachability.m Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Apple-side reachability/network detection code paths to avoid SCNetworkReachability deprecation build failures by preferring modern APIs on newer deployment targets while retaining a legacy fallback for older targets.
Changes:
- Adds a compile-time deployment-target gate (
ODW_LEGACY_REACHABILITY_REQUIRED) to exclude legacy SCNetworkReachability code from modern builds. - Refactors
NetworkInformationImpl.mmto split modern (NWPathMonitor) vs legacy (ODWReachability notification) setup paths and compile out legacy members when not needed. - Wraps multiple
ODWReachability.mcode paths with#if ODW_LEGACY_REACHABILITY_REQUIREDto avoid compiling deprecated SCNetworkReachability usage for modern targets.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| third_party/Reachability/ODWReachability.h | Introduces ODW_LEGACY_REACHABILITY_REQUIRED macro to compile out legacy reachability code on modern deployment targets. |
| third_party/Reachability/ODWReachability.m | Adds compile-time gating around legacy SCNetworkReachability branches to avoid deprecation warnings/errors on modern targets. |
| lib/pal/posix/NetworkInformationImpl.mm | Refactors network detection setup into modern (NWPathMonitor) vs legacy (ODWReachability) implementations and gates legacy members/functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Make the modern ODWReachability WWAN path explicit so iOS 12+ builds unambiguously use the NWPathMonitor-backed state, while the legacy SCNetworkReachability fallback remains only for older Apple deployment targets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bmehta001
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Modern reachability should not synchronously resolve DNS or reject the generic internet reachability address, and the path monitor context must not hold a stale raw owner pointer. Also avoid blocking the main thread while waiting for the first NWPathMonitor snapshot. Files changed: - third_party/Reachability/ODWReachability.m Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Annotate the modern-path helpers (`ODWModernPathIsReachable`, `ensureModernPathMonitor`, `awaitModernPathSnapshot`, `handleModernPathUpdate:`, `notifyModernPathChange`) with `API_AVAILABLE(macos(10.14), ios(12.0))` so that compiling against an older deployment target no longer triggers `-Wunguarded-availability-new` errors under `-Werror`. Also fix a latent bug in `isReachableViaWWAN` where the modern-path snapshot was computed before the `if (@available(...))` guard. On macOS 10.10 / iOS 10.0 deployment targets running on a host that lacks NWPathMonitor, this would invoke unavailable Network framework APIs. The modern-path branch is now only taken inside the `@available` block on legacy builds, mirroring the structure used in `isReachableViaWiFi`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`-handleModernPathUpdate:` and `-awaitModernPathSnapshot` previously read `self.initialPathSemaphore` more than once across the nil-check and the matching `dispatch_semaphore_signal`/`dispatch_semaphore_wait` call. `-stopNotifier` clears the property from an arbitrary thread, so the second read could observe nil and pass it into libdispatch (which crashes on a nil semaphore). Capture the semaphore into a strong local once. Under ARC the local retains the dispatch_semaphore_t for the duration of the call, so a concurrent stop will no longer race the signal/wait. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve host/address-specific reachability semantics with SCNetworkReachability while keeping Network.framework for general path monitoring. Avoid waiting for the first NWPathMonitor update on the monitor queue, and use platform-specific Apple availability gates. Files changed: - third_party/Reachability/ODWReachability.h - third_party/Reachability/ODWReachability.m Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Leave the Network.framework status initialization in modern monitor setup so older Apple deployment-target builds do not touch an availability-gated symbol from init. Files changed: - third_party/Reachability/ODWReachability.m Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove tvOS and watchOS deployment-target handling because the SDK does not support those targets; keep the availability gates focused on iOS and macOS. Files changed: - third_party/Reachability/ODWReachability.h - third_party/Reachability/ODWReachability.m Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reject the IPv4 unspecified wildcard (INADDR_ANY / 0.0.0.0), the IPv6 unspecified wildcard (in6addr_any / ::), and any sa_family that is not AF_INET or AF_INET6 from the public +reachabilityWithAddress: entry point. These are not routable host addresses, so creating an SC ref for them produces an ambiguous reachability probe rather than a per-host result. The legacy +reachabilityForInternetConnection fallback (only reached on deployment targets older than macOS 10.14 / iOS 12) still needs the 'probe any internet' 0.0.0.0 sentinel, so it now creates the SC ref inline and bypasses the public validator. Also document why hostname-based reachability still routes through SCNetworkReachabilityCreateWithName: NWPathMonitor has no public hostname-targeted monitoring API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+161
to
169
| // NWPathMonitor has no public hostname-targeted API: it monitors the system | ||
| // network path, not per-host reachability. Hostname-based reachability still | ||
| // routes through SCNetworkReachabilityCreateWithName, with the deprecated-API | ||
| // warning locally suppressed. The modern path-monitor backend is used by the | ||
| // hostname-agnostic isReachable* methods below. | ||
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wdeprecated-declarations" | ||
| SCNetworkReachabilityRef ref = SCNetworkReachabilityCreateWithName(NULL, [hostname UTF8String]); | ||
| SCNetworkReachabilityRef ref = SCNetworkReachabilityCreateWithName(NULL, [reachabilityHost UTF8String]); | ||
| #pragma clang diagnostic pop |
- Unconditionally set sockaddr_in::sin_len / sockaddr_in6::sin6_len in +reachabilityWithAddress: after copying the caller-supplied address. Callers (including the unit tests in ODWReachabilityTests.mm) don't reliably initialize the BSD length byte, so the previous "fix it only when zero" pattern let garbage values through and caused SCNetworkReachabilityCreateWithAddress to fail on otherwise-valid IPv4/IPv6 inputs. - Restructure +reachabilityForInternetConnection and +reachabilityForLocalWiFi so the legacy SCNetworkReachability fallback code is wrapped in #if ODW_LEGACY_REACHABILITY_REQUIRED instead of living below a modern early-return as unreachable code. This actually compiles out the deprecated SC creation APIs on modern targets, which is the stated goal of the PR, rather than relying on dead-code + pragma suppression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This URLSession-based method was the early URLSession reachability probe and is no longer referenced by anything after migrating the public reachability surface to NWPathMonitor / the legacy SCNetworkReachability notifier. Drop the stale definition so the modern Apple path doesn't carry dead code. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a header-level docstring explaining that on modern targets (iOS 12+ / macOS 10.14+), +reachabilityWithHostname: and +reachabilityWithAddress: no longer issue an NSURLSession HTTPS probe to the host's URL when -isReachable / -isReachableViaWiFi are queried. Instead they return SCNetworkReachabilityGetFlags state on the SC ref the constructor created. This is the same OS-level reachability semantics the legacy code path has always used, but it is a behavior change compared to the pre-PR modern path that performed a real HTTPS round-trip. Callers that need authoritative per-endpoint reachability should probe the endpoint themselves rather than relying on -isReachable on these instances. Document that the SDK's own production network detection (NetworkInformationImpl) uses +reachabilityForInternetConnection on legacy targets and nw_path_monitor_create() directly on modern targets, so the SDK's own behavior is unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use
NWPathMonitorfor the modern Apple reachability path while retainingSCNetworkReachabilityfor the surface-area pieces where there is no NWPathMonitor equivalent.Fixes #1425
Changes
NWPathMonitoras the primary network detection path on iOS 12+ / macOS 10.14+, with the legacyODWReachabilitynotification path retained only for older deployment targetsNWPathMonitorstate, while preserving the existing API surface and keeping the legacySCNetworkReachabilitybackend for older deployment targetsODW_LEGACY_REACHABILITY_REQUIRED) so the legacySCNetworkReachabilitynotifier path is compiled out on modern targets-[ODWReachability checkNetworkReachability:]URLSession helper that is no longer referenced after the migrationScope of the deprecation suppression
This PR moves the notifier / status query path to
NWPathMonitorso it no longer needs-Wdeprecated-declarationssuppression on modern targets. The hostname and address constructors (+reachabilityWithHostname:and+reachabilityWithAddress:) still useSCNetworkReachabilityCreateWithName/SCNetworkReachabilityCreateWithAddresswith a localized pragma suppression, becauseNWPathMonitorhas no public hostname- or address-targeted equivalent — it monitors the system path, not per-host reachability. The suppression is intentional, scoped to those three creation sites, and documented inline at the call sites.Net effect: the Xcode 26.4+ deprecation warnings that broke
-Werrorbuilds on the notifier hot path are gone, and the only remaining-Wdeprecated-declarationssuppressions are at the unavoidable SC creation sites.Behavior change for
ODWReachabilityexternal consumersThe SDK's own production network detection (
NetworkInformationImpl) only uses+reachabilityForInternetConnectionon legacy targets andnw_path_monitor_create()directly on modern targets, so the SDK's own behavior is unaffected by this PR.However, external consumers of
ODWReachability(it is a vendoredthird_party/class) who construct instances via+reachabilityWithHostname:or+reachabilityWithAddress:and then query-isReachable/-isReachableViaWiFiwill see a semantic change on modern targets:-isReachableNSURLSessionHTTPS GET to the host's URL — true per-endpoint DNS + TLS round-tripSCNetworkReachabilityGetFlagson the hostname-created SC ref — OS-level reachability flags (DNS-aware via the SC stack, but not an HTTPS probe)+reachabilityForInternetConnectionThe new per-host behavior matches what the legacy path has always done (SC flags from the hostname-created SC ref), so consumers that work today on older Apple targets will see consistent behavior on modern targets too. Documented in the header at
third_party/Reachability/ODWReachability.h.If a future caller needs an authoritative
can I reach this exact endpoint?probe, that should be added as a new method (e.g., vianw_endpoint_create_host+ a connection-style probe) rather than restoring the synchronous URLSession path that previous review iterations asked us to remove.